feat(storage): replace hummock protobuf strcut with rust struct #15386

Li0k · 2024-03-01T11:41:49Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

as title

reduce memory copy of huge struct
Easier to implement some methods for structs, less ext

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

…nto li0k/storage_pb_to_struct

wenym1 · 2024-03-05T09:37:17Z

src/storage/hummock_sdk/src/version.rs

+pub struct SstableInfo {
+    pub object_id: u64,
+    pub sst_id: u64,
+    pub key_range: Option<KeyRange>,


I think we can use FullKeyRange instead rather than the KeyRange. The field is_right_inclusive is very implicit. Maybe we can do this in a future PR.

wenym1 · 2024-03-05T09:38:30Z

src/storage/hummock_sdk/src/lib.rs

+    fn estimated_encode_len(&self) -> usize;
+}
+
+pub trait ProtoSerializeExt {


What's the difference between these two traits? May add some comments to describe the difference.

If all appearance of T is Self, we don't need the T, and we can use Self directly.

src/storage/hummock_sdk/src/table_watermark.rs

src/storage/src/hummock/compactor/compactor_runner.rs

src/storage/hummock_sdk/src/version.rs

src/meta/src/hummock/compaction/mod.rs

…nto li0k/storage_pb_to_struct

MrCroxx · 2024-03-11T06:25:24Z

src/storage/hummock_sdk/src/lib.rs

+// Similar to `ProtoSerializeExt``, we allow `ProtoSerializeOwnExt to provide a method to obtain ownership. Therefore, copying behavior in memory can be reduced
+pub trait ProtoSerializeOwnExt {
+    type PB: Clone + PartialEq + ::prost::Message;
+
+    fn from_protobuf_own(pb: Self::PB) -> Self;
+
+    fn to_protobuf_own(self) -> Self::PB;
+}


Suggested change

// Similar to `ProtoSerializeExt``, we allow `ProtoSerializeOwnExt to provide a method to obtain ownership. Therefore, copying behavior in memory can be reduced

pub trait ProtoSerializeOwnExt {

type PB: Clone + PartialEq + ::prost::Message;

fn from_protobuf_own(pb: Self::PB) -> Self;

fn to_protobuf_own(self) -> Self::PB;

}

// Similar to `ProtoSerializeExt``, we allow `ProtoSerializeOwnExt to provide a method to obtain ownership. Therefore, copying behavior in memory can be reduced

pub trait OwnedProtoSerialize {

type PB: Clone + PartialEq + ::prost::Message;

fn from_owned_protobuf(pb: Self::PB) -> Self;

fn to_owned_protobuf(self) -> Self::PB;

}

MrCroxx · 2024-03-11T06:27:18Z

src/storage/hummock_sdk/src/lib.rs

@@ -355,3 +355,25 @@ impl EpochWithGap {
        self.0 & EPOCH_SPILL_TIME_MASK
    }
 }
+
+pub trait ProtoSerializeSizeEstimatedExt {


Why are the traits all named with the suffix 'Ext'? Should they give default implementation and implements for all types that applies the trait without Ext?

Actually, I think defining a new trait is unnecessary, unless we have some common logic that takes the trait abstraction as input, but it seems that there is no such common logic. It's more like enforcing a method name convention.

MrCroxx · 2024-03-14T02:54:56Z

As discussed offline with @Li0k , it is not necessary to extract a trait for there is nowhere to use it. The structs should impl the From trait directly. e.g.

impl From<PbVnodeWatermark> for VnodeWatermark { ... }
impl From<&PbVnodeWatermark> for VnodeWatermark { ... }
impl From<VnodeWatermark> for PbVnodeWatermark { ... }
impl From<&VnodeWatermark> for PbVnodeWatermark { ... }

If there is a need to use a trait, the trait can be defined with From bound. e.g.

trait ProtoSerialize: From<&PbVnodeWatermark> + From<&VnodeWatermark> {}

And automatically impl for all types that satisfy the bound. e.g.

impl<T: From<&PbVnodeWatermark> + From<&VnodeWatermark>> ProtoSerialize for T {}

BugenZhao · 2024-03-14T03:29:30Z

The structs should impl the From trait directly.

The downside of From is that it's a foreign trait to us, which can not be further implemented on foreign types.

Say that we'll always convert Vec<usize> to repeated uint64 (which is Vec<u64> in prost) in protobuf. Currently, the developers have to manually write xx.into_iter().map(|x| x as _).collect() everywhere, which is really verbose.

It would be nice if we can directly write xx.into(). However, this is not possible if we're leveraging the From trait as there's no way to implement something like impl From<Vec<usize>> for Vec<u64>. Instead, a custom trait will enable this and further make it possible to directly derive the implementation by recursively visiting the fields on complex messages.

MrCroxx · 2024-03-14T03:49:24Z

The structs should impl the From trait directly.

The downside of From is that it's a foreign trait to us, which can not be further implemented on foreign types.

Say that we'll always convert Vec<usize> to repeated uint64 (which is Vec<u64> in prost) in protobuf. Currently, the developers have to manually write xx.into_iter().map(|x| x as _).collect() everywhere, which is really verbose.

It would be nice if we can directly write xx.into(). However, this is not possible if we're leveraging the From trait as there's no way to implement something like impl From<Vec<usize>> for Vec<u64>. Instead, a custom trait will enable this and further make it possible to directly derive the implementation by recursively visiting the fields on complex messages.

Do we need to impl From for low-level type like Vec<usize>? I thought impl on defined message types is enough before. 🤔

…nto li0k/storage_pb_to_struct

wenym1 · 2024-03-09T07:50:25Z

src/meta/src/hummock/compaction/picker/min_overlap_compaction_picker.rs

@@ -326,7 +325,7 @@ impl NonOverlapSubLevelPicker {
            level_ssts.sort_by(|sst1, sst2| {
                let a = sst1.key_range.as_ref().unwrap();
                let b = sst2.key_range.as_ref().unwrap();
-                a.compare(b)
+                a.cmp(b)


May use sort_by_key here?

Due to the design of interface, we should clone the key_range if we use sort_by_key. Even though copying the key_range after the PR is lightweight, I wanted to keep the clone from being triggered as much as possible.

src/storage/hummock_sdk/src/key_range.rs

src/storage/hummock_sdk/src/change_log.rs

src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

wenym1 · 2024-05-11T06:00:03Z

src/storage/hummock_sdk/src/lib.rs

@@ -355,3 +355,25 @@ impl EpochWithGap {
        self.0 & EPOCH_SPILL_TIME_MASK
    }
 }
+
+pub trait ProtoSerializeSizeEstimatedExt {


Actually, I think defining a new trait is unnecessary, unless we have some common logic that takes the trait abstraction as input, but it seems that there is no such common logic. It's more like enforcing a method name convention.

src/storage/hummock_test/src/hummock_read_version_tests.rs

src/meta/src/hummock/compaction/mod.rs

wenym1 · 2024-05-13T05:31:20Z

src/storage/hummock_sdk/src/version.rs

+    pub uncompressed_file_size: u64,
+}
+
+impl From<&PbOverlappingLevel> for OverlappingLevel {


I think it's unnecessary to implement a trait for the conversion, because we don't have any code that depends on the From or Into trait. The only benefit is that when we implement From, we can call .into(), but the benefit is limited. The code will be the same if we don't implement the trait and just implement a method for the new struct like

impl OverlappingLevel { // No impl `From<&PbOverlappingLevel>` fn from(pb_overlapping_level: &PbOverlappingLevel) -> Self { Self { ... } } }

And then we'd better rename the method name from from, which is too general, to from_protobuf and to_protobuf so that we can explicitly know that we are doing a conversion between new struct and pb when we read the code.

ditto for all other impl From in this PR.

I'm not biased. @hzxa21

In some cases, I want to consume the incoming arguments directly (instead of copying them), and if we want to fulfil this requirement, we still need to implement four functions

from_protobuf(struct)

from_protobuf(struct_ref)

to_protobuf1(struct)

to_protobuf2(struct_ref)

I don't think there's much difference except in naming, as in commit, and I've provided from_protobuf and to_protobuf wrappers for some common structures

…nto li0k/storage_pb_to_struct

gitguardian · 2024-07-11T13:34:00Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`ebcc00b`	ci/scripts/e2e-sink-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

wenym1

Rest LGTM

src/storage/hummock_sdk/src/version.rs

src/meta/src/hummock/compaction/mod.rs

src/storage/hummock_sdk/src/table_watermark.rs

src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

src/storage/hummock_sdk/src/version.rs

wenym1 · 2024-07-14T09:37:39Z

src/storage/hummock_sdk/src/version.rs

+}
+
+impl CompactTask {
+    pub fn task_type(&self) -> PbTaskType {


Ditto for these methods.

Besides, I think we should not provide method to get the PbXxx type from the CompactTask. We'd better just return the the Xxx defined by ourselves.

I will change it in other PR, remove the get function first

src/storage/hummock_sdk/src/version.rs

src/meta/service/src/hummock_service.rs

…nto li0k/storage_pb_to_struct

wenym1

Rest LGTM! Thanks for the great work!

src/meta/model_v2/src/hummock_sstable_info.rs

src/meta/src/hummock/manager/compaction.rs

src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

wenym1 · 2024-07-24T16:37:09Z

src/storage/hummock_sdk/src/version.rs

+                .sum::<usize>()
+            + size_of::<u32>()
+    }
+}


Can we split this file and move the new structs to separate files to avoid making this file too large?

…nto li0k/storage_pb_to_struct

feat(storage): replace pb with struct

1f30ff7

github-actions bot added the type/feature label Mar 1, 2024

Li0k added 2 commits March 4, 2024 15:59

fix(storage): fix check

163ff20

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

e866deb

…nto li0k/storage_pb_to_struct

Li0k requested a review from Little-Wallace March 4, 2024 08:00

Li0k marked this pull request as ready for review March 4, 2024 08:01

Li0k requested a review from a team as a code owner March 4, 2024 08:01

Li0k changed the title ~~feat(storage): replace pb with struct~~ feat(storage): replace hummock protobuf strcut with rust struct Mar 4, 2024

Li0k mentioned this pull request Mar 4, 2024

Tracking: optimize performance of hummock manager in meta-node #15397

Closed

3 tasks

Li0k added 2 commits March 4, 2024 18:43

refactor(storage): refactor and reduce copy

940b233

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

9a8ebbc

…nto li0k/storage_pb_to_struct

Li0k requested a review from wenym1 March 4, 2024 10:44

wenym1 reviewed Mar 5, 2024

View reviewed changes

Li0k added 2 commits March 8, 2024 15:37

fix(storage): address comments

afd7469

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

9f9707e

…nto li0k/storage_pb_to_struct

Li0k requested review from MrCroxx and hzxa21 March 8, 2024 07:59

Li0k added 3 commits March 8, 2024 18:19

fix(storage): fix compile

3bf2bf3

fix(storage): fix check

4f72cba

fix(storage): fix check

0a75e90

MrCroxx reviewed Mar 11, 2024

View reviewed changes

Li0k added 3 commits May 7, 2024 17:40

refactor(storage): refactor version

5672d61

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

1758269

…nto li0k/storage_pb_to_struct

fix(storage): fix compile

480fc62

github-actions bot added the ci/run-e2e-single-node-tests label May 9, 2024

Li0k added 2 commits May 9, 2024 18:03

fix(storage): fix check

5fff8ae

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

b912567

…nto li0k/storage_pb_to_struct

wenym1 reviewed May 13, 2024

View reviewed changes

graphite-app bot requested a review from a team June 19, 2024 04:34

risingwavelabs deleted a comment from graphite-app bot Jun 19, 2024

BugenZhao removed the request for review from a team June 19, 2024 04:37

Li0k added 2 commits July 11, 2024 19:42

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

ebcc00b

…nto li0k/storage_pb_to_struct

fix(storage): address comments

c5d4b7f

fix(storage): address check

7a1fbb1

wenym1 reviewed Jul 15, 2024

View reviewed changes

Li0k added 5 commits July 23, 2024 21:00

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

705f7b0

…nto li0k/storage_pb_to_struct

fix(storage): address comments

bf1addf

fix(storage): fix compile

a22660b

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

ee43cd3

…nto li0k/storage_pb_to_struct

fix(storage): fix sync_point and bench code

75d72d4

wenym1 approved these changes Jul 24, 2024

View reviewed changes

Li0k added 2 commits July 25, 2024 14:59

fix(storage): reorg and address comments

2de6a48

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

7eae479

…nto li0k/storage_pb_to_struct

Li0k enabled auto-merge July 25, 2024 07:01

Li0k added 3 commits July 25, 2024 15:28

fix(storage): fix compile

6018f58

fix(storage): fix test compile

ffc5ac6

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

66011da

…nto li0k/storage_pb_to_struct

Li0k added this pull request to the merge queue Jul 25, 2024

Merged via the queue into main with commit 1c2c400 Jul 25, 2024
30 of 31 checks passed

Li0k deleted the li0k/storage_pb_to_struct branch July 25, 2024 08:28

zwang28 mentioned this pull request Oct 18, 2024

fix(meta): read correct id field #18988

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): replace hummock protobuf strcut with rust struct #15386

feat(storage): replace hummock protobuf strcut with rust struct #15386

Li0k commented Mar 1, 2024 •

edited

Loading

wenym1 Mar 5, 2024 •

edited

Loading

wenym1 Mar 5, 2024

wenym1 Mar 5, 2024

Li0k Mar 8, 2024

MrCroxx Mar 11, 2024 •

edited

Loading

MrCroxx Mar 11, 2024

wenym1 May 11, 2024

MrCroxx commented Mar 14, 2024

BugenZhao commented Mar 14, 2024

MrCroxx commented Mar 14, 2024

wenym1 Mar 9, 2024

Li0k Jul 11, 2024

wenym1 May 11, 2024

wenym1 May 13, 2024

Li0k Jul 11, 2024

Li0k Jul 11, 2024

gitguardian bot commented Jul 11, 2024 •

edited

Loading

wenym1 left a comment

wenym1 Jul 14, 2024

Li0k Jul 23, 2024

wenym1 left a comment

wenym1 Jul 24, 2024

feat(storage): replace hummock protobuf strcut with rust struct #15386

feat(storage): replace hummock protobuf strcut with rust struct #15386

Conversation

Li0k commented Mar 1, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

wenym1 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCroxx Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCroxx commented Mar 14, 2024

BugenZhao commented Mar 14, 2024

MrCroxx commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Jul 11, 2024 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Mar 1, 2024 •

edited

Loading

wenym1 Mar 5, 2024 •

edited

Loading

MrCroxx Mar 11, 2024 •

edited

Loading

gitguardian bot commented Jul 11, 2024 •

edited

Loading